-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark chart.spec.version as specific to HelmRepository source in example #709
base: main
Are you sure you want to change the base?
Conversation
Thanks for your feedback @tjanson ! I am always looking for ways to make the Helm Controller docs more accessible. I'm not sure if this is the change either, but it might be a step in the right direction. If it is to be merged, please sign off your commit as described in the failing DCO merge check: https://github.com/fluxcd/website/pull/709/checks?check_run_id=4881437581 What people usually struggle more with, IMHO, is that GitRepository sources aren't using the Maybe somewhere in the "Manage Helm Releases" guide, we can add something about this too. It's good to shout out all of the ways that Helm Controller can behave differently when it is used with a |
Indeed, I just learned about that too. Anything that increases visibility of the fact that Has this behavior been brought up as an issue before? I assumed it was likely already discussed and decided. |
In my opinion it is not visible enough that `chart.spec.version` is only considered for `HelmRepository` sources (and silently ignored otherwise. I'm aware that the text below the example mentions this, but I think it would be benefitial for the code example itself to make that fact clear. Adding this comment is only a suggestion (which I'm not 100% happy with myself) -- feel free to substitute a different way of pointing out the connection between `HelmRepository` and the version spec. Signed-off-by: Tom Janson <[email protected]>
No I don't think anyone has ever raised this issue before, I would suggest that you file it separately on the helm-controller. I'm sure that ignoring the version is desired behavior, since the only thing that can effectively be deployed from is the thing at the HEAD of the repository ref. What else is surprising though is that What's worse is that if you think about it hard enough, these are both the least surprising behavior, given the medium. But, accepting a I actually feel like that's a good "read the docs" trigger action – if the user passes in a HelmRelease with a I'm absolutely not saying we've definitely covered everything as well as we can already in the docs, I just do think there is a decent chance that building the widest funnel (shunting people toward docs most effectively when it's most needed) is as important if not more than getting the actual content of the docs right. 👍 |
In my opinion it is not visible enough that
chart.spec.version
is only considered forHelmRepository
sources (and silently ignored otherwise.I'm aware that the text below the example mentions this, but I think it would be benefitial for the code example itself to make that fact clear.
Adding this comment is only a suggestion (which I'm not 100% happy with myself) -- feel free to substitute a different way of pointing out the connection between
HelmRepository
and the version spec.More generally, have you considered throwing an error when
chart.spec.version
is specified with e.g. aGitSource
containing a different version of the chart (rather than using it despite the version mismatch)? The current behavior is not what I personally would expect.